[FC-0118] docs: Add ADR for ensuring GET requests are idempotent#38249
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @taimoor-ahmed-1! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
@bmtcril I'd love to get your thoughts on this. @taimoor-ahmed-1 while I'm not generally opposed to idempotent GET requests, I think an exception for telemetry data should exist. Pushing that to async calls increases cost of what should be low overhead calls. |
|
I think there is a difference between GETs that side-effect the domain or transactional state and those that trigger changes in a different domain. Analytics and other telemetry are separate domains from, say, student state and aren't modifying any data relevant to the GET request, which is what the distinction is really about. I totally agree with things like the student state not being modified by side-effects in GET requests, but I think logging and telemetry are separate concerns. |
taimoor-ahmed-1
force-pushed
the
docs/ADR-ensure_get_requests_idempotent
branch
from
9f4a500 to
e581b94
Compare
@feanil @bmtcril I have updated the ADR based on your feedback |
taimoor-ahmed-1
force-pushed
the
docs/ADR-ensure_get_requests_idempotent
branch
from
0e4ff39 to
8f49e9e
Compare
bmtcril
left a comment
bmtcril
left a comment
There was a problem hiding this comment.
Looks good to me. It seems like the CI error is a temporary coverage issue, you might need to close and reopen the PR to trigger another run unfortunately
taimoor-ahmed-1
force-pushed
the
docs/ADR-ensure_get_requests_idempotent
branch
from
8f49e9e to
40c8353
Compare
| Decision | ||
| ======== | ||
|
|
||
| 1. Treat ``GET`` as strictly read-only with respect to **domain state**: a ``GET`` handler |
There was a problem hiding this comment.
I think it's worth elaborating on what we mean by domain state here. We have a lot of good conversation in comments but that won't be available to users when they're reading the ADR so we should capture the nuance around tracking logs and other non-domain writes and document clearly that we think that's okay. That's one of the pieces of decision that we agreed to from this discussion.
There was a problem hiding this comment.
Added the description
| Some Open edX endpoints use ``GET`` requests that have side-effects (e.g., firing | ||
| openedx-events, triggering Django signals, writing tracking logs, recording first access | ||
| events). This violates REST safety/idempotency expectations and can break | ||
| caching/proxy behavior and automated clients/agents. |
There was a problem hiding this comment.
This context paragraph needs to change now that we've made the distinction between domain state and non domain state writes.
Add edx-platform/docs/decisions/0030-ensure-get-requests-are-idempotent.rst as an accepted ADR. Define policy that GET endpoints must be strictly read-only, with side effects moved to explicit write endpoints or async event pipelines. Include edx-platform relevance, anti-pattern vs preferred code examples, and rollout guidance for testing and migration.
taimoor-ahmed-1
force-pushed
the
docs/ADR-ensure_get_requests_idempotent
branch
from
40c8353 to
cae49eb
Compare
| ======= | ||
|
|
||
| Some Open edX endpoints use ``GET`` requests that mutate **domain state** as a | ||
| side-effect — for example, firing openedx-events, triggering Django signals, or |
There was a problem hiding this comment.
@felipemontoya @mariajgrimaldi I want to get your thoughts on the idea of not firing openedx-events in GET requests? Based on your experience, do you think this is reasonable?
@taimoor-ahmed-1 one reason that I can think of for still wanting to fire some sort of signal is to trigger logging or metrics capture in a distributed way. It may be sufficient to say that a GET shouldn't violate idempotency either directly or via events/signals/etc.
There was a problem hiding this comment.
To my recollection we don't fire events when doing GET requests. Looking at the learning domain which is the one with the longest list of events, they all happen after something was changed in the state and it is completely reasonable that should happen in a POST or PATCH.
Now, during GET requests, what we do fire are openedx-filters. These are intended to be fired on GET, because part of the requirements are that we run extension points to add more info to a GET, remove objects from a List or these sort of operations. Now, by firing the filter, we delegate to implementers of the pipeline steps what they will do. There is no easy way to block a pipeline step from altering something.
9 participants
Description
This PR adds ADR-0030 to document and standardize idempotent GET behavior across Open edX REST APIs. The ADR establishes that GET must be read-only and that any state mutations or tracking writes should be handled via explicit write endpoints or decoupled async telemetry.
What changed
Added edx-platform/docs/decisions/0030-ensure-get-requests-are-idempotent.rst
Context on current non-idempotent GET risks
Decision and required implementation rules
edx-platform-specific relevance notes
Code examples showing anti-pattern and preferred approach
related issue: #38248